Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#4478] New notes model and admin controller #7212

Merged
merged 5 commits into from
Aug 30, 2022
Merged

[#4478] New notes model and admin controller #7212

merged 5 commits into from
Aug 30, 2022

Conversation

gbp
Copy link
Member

@gbp gbp commented Aug 10, 2022

Relevant issue(s)

Fixes #4478
Fixes #6994

What does this do?

Adds new notes model and admin controller. Associated as a "concrete note" with PublicBody via a new Notable concern. Also can be assoicated as a "tagged note" via a notable_tag column.

Why was this needed?

Easier handling of notes, especially via tags, so notes don't need to be manually added to multiple authorities.

Implementation notes

Have extract out these new issues from #4478

These will be done as separate PRs.

Screenshots

Notes to reviewer

This adds admin route helpers for `PublicBody` and `InfoRequest` so we
can call:
1. `admin_public_body_path(...)` -> `/admin/bodies/123`
2. `admin_info_request_path(...)` -> `/admin/requests/123`
3. `url_for([:admin, public_body_or_info_request])` and the correct
   route returned.

This will be helpful for redirecting after creating/updating notes for a
given record.
@gbp gbp changed the base branch from 6992-tag-admin to develop August 11, 2022 12:05
@gbp gbp changed the title 4478 notes [#4478] New notes model and admin controller Aug 11, 2022
gbp added 3 commits August 11, 2022 13:08
Instead of needing a `HasTagString::HasTagStringTag` instance this
commit changes the helper to also accept a text string.

Will correctly link and style the string tag with name/value to the tag
admin controller.

As this doesn't include the `model_type` so the tag controller will
assume `PublicBody` which is fine and a good assumption to make.
Adds an association as `Notable#concrete_notes` to avoid clashing with
the current `PublicBody#notes` column.

This uses a polymorphic association so any model can be `Notable`.
@gbp gbp marked this pull request as ready for review August 23, 2022 07:31
Copy link
Member

@garethrees garethrees left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good in principle; admin ui seems to break so can't test fully

@@ -485,6 +485,22 @@
end
####

#### AdminNote controller
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

be nicer to have shallow routes for new so that we can do e.g. /admin/bodies/3/notes/new (which automatically sets notable_type & notable_id), but then editing the created note is simply /admin/note/:id/edit. not essential this minute though

app/views/admin/tags/show.html.erb Outdated Show resolved Hide resolved
@gbp gbp requested a review from garethrees August 25, 2022 12:23
@gbp gbp removed their assignment Aug 25, 2022
Allow notes to be created with an association to tags via a
`Note#notable_tag` column.

This updates `Notable#all_notes` to combine concrete and tagged notes.

Fixes #6994
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow tags to have notes Extract PublicBody#notes to model
2 participants